fix: don't scan the whole table to discover Map keys by niladrix719 · Pull Request #3082 · ClickHouse/ClickStack

niladrix719 · 2026-09-04T13:21:18Z

Summary

getMapKeys only added a time filter when the caller happened to pass both a date range and a timestamp expression. Several UI autocomplete call sites (chart editor, alert modal, dashboard filters) passed neither. When that happened, it fell back to an unbounded scan across the whole table instead of skipping or narrowing the query

Steps to reproduce

open the chart editor, don't touch the time picker, and start typing a Group By expression on a Map column (ResourceAttributes[). that keystroke sent ClickHouse a query with no time filter, reading every part of the table, not just recent ones

The fix

Both the raw-table scan and the text-index lookup now refuse to run without something to bound them. Field autocomplete falls back to a 24h window when it
knows the timestamp column but not the range, and the call sites that had a source/date range in scope but weren't passing them now do

Before / after

Before:

SELECT token AS key FROM mergeTreeTextIndex('default', 'otel_logs', 'idx_res_attr_key')
WHERE 1
GROUP BY key HAVING key != ''
LIMIT 1000
FORMAT JSON

No WHERE/predicate, full-table scan

After:

SELECT token AS key FROM mergeTreeTextIndex('default', 'otel_logs', 'idx_res_attr_key')
WHERE part_name IN (
  SELECT name FROM system.parts
  WHERE database = 'default' AND table = 'otel_logs' AND active = 1
    AND (min_time >= fromUnixTimestamp64Milli(...) AND min_time <= fromUnixTimestamp64Milli(...))
       OR ...
)
GROUP BY key HAVING key != ''
LIMIT 1000
FORMAT JSON

Known gap

The SQL editors in source-configuration forms still don't pass scope, so Map keys won't autocomplete there. Left as a follow-up, needs the form's in-progress timestampValueExpression, which isn't available the same way

changeset-bot · 2026-09-04T13:21:24Z

🦋 Changeset detected

Latest commit: 812939d

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2026-09-04T13:21:25Z

@niladrix719 is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

greptile-apps · 2026-09-04T13:24:47Z

Confidence Score: 5/5

[Medium risk] Adds date range defaults to metadata discovery queries.

The PR appears safe to merge; no new actionable issue was established in the changes since the previous review.

Summary

This PR bounds Map-key discovery when timestamp context is available and passes source context through autocomplete callers. The changes since the previous review also adapt event-delta selection queries for Distributed and Merge tables.

Local tables retain the PartIds hint; pointer tables use GLOBAL IN and omit the shard-local hint.
Tests cover both selection paths and the metadata-lookup fallback.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Event-delta selection] --> B[Look up table metadata]
  B --> C{Confirmed local table?}
  C -->|Yes| D[Use IN and PartIds hint]
  C -->|No or lookup failed| E[Use GLOBAL IN without PartIds hint]

_{Reviews (61) · Last reviewed commit: "Merge branch 'main' into fix-scan-mapkey..."}

github-actions · 2026-09-04T13:27:54Z

Deep Review

Scope: PR #3082 — bound Map-key autocomplete discovery to a time window (#3037). 16 files, ~446 insertions across the merge-base range 3876d6b9..HEAD (6 commits). Core change is getMapKeys in packages/common-utils/src/core/metadata.ts; the rest threads sourceId/dateRange/timestampValueExpression into autocomplete call sites.

✅ No critical issues found. The fix is sound for the wired call sites: the text-index paths now refuse to run without a timestampValueExpression, the raw scan gains a time filter when one is present, defaultFieldMetadataDateRange() rounds the end up (so current-hour keys are not hidden), and the primary cache key is deliberately kept range-independent when there is no timestamp expression to prevent TTL-less cache churn. No P0/P1 defects are introduced by this diff.

🟡 P2 — recommended

packages/common-utils/src/core/metadata.ts:1103 — When timestampValueExpression is absent the raw main-table scan still runs with no time predicate (only a console.warn), so any caller lacking a timestamp expression continues issuing the unbounded discovery read the PR targets; this is the acknowledged known-gap path (source-config SQL editors), not a regression, but it leaves the stated protection incomplete.
- Fix: Return [] (or require a bounding predicate) instead of running the unfiltered scan when no timestampValueExpression is available, mirroring the text-index guard.
packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx:238 — The new sourceId/dateRange propagation across the chart, alert, dashboard-filter, and heatmap editors has no test proving each editor resolves its source and produces bounded Map-key suggestions; the hook tests mock getAllFields, so broken source/table wiring would pass silently.
- Fix: Add a focused UI or e2e test covering the source-only and explicit-range autocomplete paths for at least one chart editor and the alert/dashboard-filter editors.

🔵 P3 nitpicks (2)

packages/common-utils/src/core/metadata.ts:1043 — The rollup fallback's .rollup-suffixed cache key carries the aligned range, so on the default rolling window it rotates every granularity bucket and accumulates empty Map-column rollup entries in the TTL-less cache over a long-lived client session.
- Fix: Skip the rollup query for Map columns, or use a stable bucket when the caller passes no range/timestamp expression.
packages/common-utils/src/core/metadata.ts:973 — The two mergeTreeTextIndex queries omit abort_signal, so an abandoned autocomplete keeps running server-side until it finishes (the raw scan already forwards the signal); pre-existing, but adjacent to the changed code.
- Fix: Pass abort_signal: signal to both text-index query calls.

Reviewers (5): correctness, testing, typescript, performance, previous-comments.

Testing gaps: New UI call-site wiring (sourceId/dateRange threading) is untested end-to-end; the metric-name cache-key path and the getAllFields timestamp-expression addition rely on unit tests with mocked ClickHouse rather than integration coverage.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

github-actions · 2026-09-04T14:06:21Z

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.

🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.

Anything about surrounding code, or asking you to widen the change (hoist a helper,
dedupe with another file, fix other call sites): never fix here, whatever the
severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

5 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 4 minor

4 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

🟠 packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:228 — Raw SQL editor's metric and materialized-view table connections still have no timestampValueExpression, so their Map key scans stay unbounded → getAllMetricTables (packages/app/src/utils.ts:1276) and the MV entries at lines 239-243 build TableConnections without timestampValueExpression, and SQLEditor passes no override. Every Map column on the metric gauge/sum/histogram tables therefore still hits the unbounded raw-scan path this PR is meant to close. Set timestampValueExpression: source.timestampValueExpression in getAllMetricTables and on the MV connections, or list this in the PR's known gaps alongside the source forms.

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

github-actions · 2026-09-25T20:40:05Z

+        `Skipping Map key discovery for ${databaseName}.${tableName}.${column}: no timestampValueExpression to bound the scan`,
+      );
+      // Cache so getAllFields doesn't re-warn per Map column on every call.
+      this.cache.set(cacheKey, []);


🟠 major — Skip branch caches [] under a key that omits timestampValueExpression on the metadataMVs path

When metadataMVs is set, cacheKey (lines 924-925) is built only from the aligned range and leaves out timestampValueExpression. The new skip branch then writes [] to that key. Suppose a log or trace source has MVs whose rollup is empty for Map columns, which the PR's own comment says is the norm. If an editor gets tcFromSource(source) without a sourceId and so calls first with no timestamp expression, every later call for the same column and range that does pass the expression hits the cached [] and never reaches the bounded scan. MetadataCache (line 206) has no TTL, so this lasts until the page reloads. Fix: add timestampValueExpression to the MV-path key, or return [] from the skip branch without writing it to the shared cacheKey. Add a test that calls without and then with the expression.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

github-actions · 2026-09-25T20:40:05Z

    inlineNonNegativeInt(maxKeys, 'maxKeys');

+    // Default a missing range instead of scanning unbounded. #3037
+    const dateRange = callerDateRange ?? defaultFieldMetadataDateRange();


🔵 minor — Only the defaulted range is hour-aligned, so every caller-supplied range runs a fresh raw scan

defaultFieldMetadataDateRange is hour-aligned so the cache rotates hourly, but a callerDateRange goes into dateRangeCacheSuffix and timeFilterExpr unaligned on the non-MV path. This PR now passes the chart editor's dateRange into many editors, so every time-picker change or relative-range refresh gives a new cache key and a new main-table scan. Before, those editors hit one cached entry. Fix: align callerDateRange too, e.g. getAlignedDateRange(callerDateRange, '1 hour'), before building the key and filter.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

github-actions · 2026-09-25T20:40:05Z

+const FIELD_METADATA_LOOKBACK_MS = 24 * 60 * 60 * 1000;
+
+// Hour-aligned so a range-keyed cache rotates hourly instead of every call.
+export function defaultFieldMetadataDateRange(): [Date, Date] {


🔵 minor — defaultFieldMetadataDateRange is exported but has no callers outside this file

Grep shows the only use is getMapKeys at line 914 (the tests don't import it either). Drop the export, or use it in describeSource.ts in place of its hand-rolled VALUE_SAMPLE_LOOKBACK_MS range so there is one default.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

github-actions · 2026-09-28T13:41:21Z

-      dateRange && timestampValueExpression
-        ? `${dateRange[0].getTime()}-${dateRange[1].getTime()}-${timestampValueExpression}`
-        : '';
+    const dateRangeCacheSuffix = `${dateRange[0].getTime()}-${dateRange[1].getTime()}-${timestampValueExpression ?? ''}`;


🟠 major — Map key cache key now includes a date range the query ignores when there is no timestampValueExpression, so the full-table scan re-runs

When timestampValueExpression is missing, the scan has no time filter, so its result doesn't depend on dateRange. The cache key still includes it. Before this PR the suffix was '' in this case and the key never changed. Now the defaulted hour-aligned range gives a new key every hour, and a caller-supplied range gives a new key on every change. Each new key re-runs the full-table scan, and MetadataCache has no TTL or eviction, so entries also pile up. Fix: use a suffix without the range when timestampValueExpression is unset, e.g. timestampValueExpression ? ${dateRange[0].getTime()}-${dateRange[1].getTime()}-${timestampValueExpression} : '', and do the same for the alignedDateRange key.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

vercel · 2026-09-28T14:27:09Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Sep 28, 2026 2:42pm UTC
hyperdx-storybook	Ready	Preview	Sep 28, 2026 2:42pm UTC

niladrix719 · 2026-09-28T14:52:35Z

@knudtty Thanks for the review, I saw you made some additional improvements as well. Thanks for taking the time to do that

github-actions Bot added the external label Sep 4, 2026

greptile-apps Bot reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/common-utils/src/core/metadata.ts Resolved

Comment thread packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx Resolved

niladrix719 force-pushed the fix-scan-mapkeys#3037 branch from a3fb411 to 02b87c1 Compare September 4, 2026 13:34

greptile-apps Bot reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/common-utils/src/__tests__/metadata.test.ts Resolved

greptile-apps Bot reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/api/src/controllers/__tests__/ai.test.ts Resolved

niladrix719 force-pushed the fix-scan-mapkeys#3037 branch 2 times, most recently from 1712d3d to bc6864f Compare September 4, 2026 13:50

greptile-apps Bot reviewed Sep 4, 2026

View reviewed changes

Comment thread packages/api/src/controllers/__tests__/ai.test.ts Resolved

niladrix719 force-pushed the fix-scan-mapkeys#3037 branch from bc6864f to fc153e3 Compare September 4, 2026 13:56